-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
added lhn-backbutton #44753
added lhn-backbutton #44753
Conversation
@rushatgabhane Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
I've been OOO so might have missed something but this recording seems wrong to me. If you navigate to: Instead I'd expect back to show the previous page and LHN which in this instance would be Unless I'm very confused. cc @Expensify/design |
@dubielzyk-expensify yeah that's right it should take you to the hub |
@Taiwrash please fix it |
Totally agree @dubielzyk-expensify - I feel like I had similar comments about this back button behavior elsewhere, I can't remember where though. |
Ah yes, @Taiwrash please take another look at the comments I had on your closed PR here. |
Oh! I can't actually remember where exactly I got the suggestion to make it the way I did before. Below is the update made
Screen.Recording.2024-07-03.at.07.32.02.mov |
I think that seems better, thanks! |
Hello everyone, I am still awaiting our feedback to merge this @rushatgabhane @dubielzyk-expensify @shawnborton |
That looks good to me too. Can you confirm that the same back behaviour also happens with the back button on the browser? |
Here is the demo showing the browser back button Screen.Recording.2024-07-07.at.01.29.14.mov |
That looks and feels pretty good to me. |
@rushatgabhane all corrections made |
Looking forward to the next review @rushatgabhane @shawnborton @dubielzyk-expensify @r8 |
@rushatgabhane just fixed all |
Still await the final review for this, I want to pick another issue to work on |
@rushatgabhane @dubielzyk-expensify @r8 when is the next review please? |
@@ -49,27 +49,41 @@ function isInRange(num, min, max) { | |||
* If a page is directly accessed (e.g., via deep link, bookmark, or opened in a new tab),the user will be navigated | |||
* back to the relevant hub page of that article. | |||
*/ | |||
function navigateBack() { | |||
const currentHost = window.location.host; | |||
const referrer = document.referrer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Taiwrash do you know why this code was removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Taiwrash bug 1: clicking any item in LHN takes you back
Screen.Recording.2024-07-23.at.04.05.56.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug 2: if you click back
from LHN, you can't go forward using browser forward button
@Taiwrash were you able to fix the bugs? |
What's the latest on this one? |
Friendly bump, do we close this one or are we still doing it? |
@shawnborton we can close this PR. This was completed by another contributor in another PR @dannymcclain from design reviewed it |
Sounds good, will close. |
Details
back button
is clicked. I updated thenavigateBack
function.Fixed Issues
PROPOSAL:
$ #42600
$ #42600 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Screen.Recording.2024-07-03.at.01.03.53.mov